Skip to content

Added error message when using --noLib and not importing any builtins at all #190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Added error message when using --noLib and not importing any builtins at all #190

wants to merge 1 commit into from

Conversation

fcrick
Copy link
Contributor

@fcrick fcrick commented Jul 29, 2018

When you compile with --noLib and you don't import anything, the compiler gets confused because the required builtin function start is not available. It wasn't really clear to me if that needed to be addressed, but it seemed like making the error more clear about how to fix the problem would be a good start.

Seems like it should also say so if compilation fails, instead of saying "Cannot read property 'setOptimizeLevel' of undefined", but this change doesn't address that.

New first line of error output:

ERROR: AssertionError: builtin "start" missing. Try adding: import "builtins"

Existing error output:

ERROR: AssertionError: assertion failed
    at assert (C:\src\assemblyscript\std\portable\index.js:172:9)
    at Program.initialize (C:\src\assemblyscript\src\program.ts:623:21)
    at Compiler.compile (C:\src\assemblyscript\src\compiler.ts:320:13)
    at Object.compileProgram (C:\src\assemblyscript\src\index.ts:150:41)
    at stats.compileTime.measure (C:\src\assemblyscript\cli\asc.js:453:33)
    at measure (C:\src\assemblyscript\cli\asc.js:809:3)
    at C:\src\assemblyscript\cli\asc.js:452:28
    at Object.main (C:\src\assemblyscript\cli\asc.js:458:5)
    at Object.<anonymous> (C:\src\assemblyscript\bin\asc:3:60)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
C:\src\assemblyscript\node_modules\binaryen\index.js:5
if(n){var da,ea;a.read=function(b,e){var c=q(b);c||(da||(da=require("fs")),ea||(ea=require("path")),b=ea.normalize(b),c=da.readFileSync(b));return e?c:c.toString()};a.readBinary=function(b){b=a.read(b,!0);b.buffer||(b=new Uint8Array(b));assert(b.buffer);return b};1<process.argv.length&&(a.thisProgram=process.argv[1].replace(/\\/g,"/"));a.arguments=process.argv.slice(2);process.on("uncaughtException",function(b){if(!(b instanceof fa))throw b;});process.on("unhandledRejection",function(){process.exit(1)});



                                      ^

TypeError: Cannot read property 'setOptimizeLevel' of undefined
    at Object.main (C:\src\assemblyscript\cli\asc.js:495:10)
    at Object.<anonymous> (C:\src\assemblyscript\bin\asc:3:60)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:236:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:560:3)

@dcodeIO
Copy link
Member

dcodeIO commented Jul 29, 2018

What do you think of including the builtins file even in the --noLib case instead? Or would that already provide "too much", and there should instead be an alternative more minimal builtins file that becomes included?

@fcrick
Copy link
Contributor Author

fcrick commented Jul 29, 2018

I think including the builtins even with --noLib is ok with me. I was a bit confused by having start mixed in there, I guess, but if it's auto-included, that won't be an issue.

@dcodeIO
Copy link
Member

dcodeIO commented Jul 29, 2018

Yeah, this exists because start needs to have a declaration somewhere to be a valid AST node, in case a diagnostic references it, i.e. when declaring a duplicate global start function.

@fcrick
Copy link
Contributor Author

fcrick commented Jul 29, 2018

i guess a few of them seem like they could be somewhere else, like:

HEAP_BASE
__gc_iterate_roots

and maybe

ERROR
WARNING
INFO

My preference would be to split up things based on what they depend on, in general.

@dcodeIO
Copy link
Member

dcodeIO commented Jul 29, 2018

ERROR, WARNING, INFO etc. could go to diagnostics.ts or something. But without HEAP_BASE, one won't be able to implement any sort of dynamic memory management when static data, i.e. strings, are present. __gc_iterate_roots will go away (moved to gc.ts) once the GC branch is ready.

@fcrick
Copy link
Contributor Author

fcrick commented Jul 29, 2018

But without HEAP_BASE, one won't be able to implement any sort of dynamic memory management when static data, i.e. strings, are present.

That sounds like it should declared (or referenced) by those types, since its their dependency. Nothing else in builtins depends on HEAP_BASE, correct?

@dcodeIO
Copy link
Member

dcodeIO commented Jul 29, 2018

Hmm, yeah, now that you mention it I think it could be moved to memory.ts.

@dcodeIO dcodeIO closed this in 658ab23 Sep 9, 2018
@dcodeIO
Copy link
Member

dcodeIO commented Sep 9, 2018

Things should be properly moved now, and the builtins file should always be included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fcrick @dcodeIO